Make sure JSON parser returns Hash #4106

daipom · 2023-03-21T02:13:33Z

Which issue(s) this PR fixes:
Fixes #4100

What this PR does / why we need it:
Make sure JSON parser returns Hash.
It is wrong for Parser to return a record that is not Hash.
Subsequent processing may result in errors.

The result of parsing JSON can be a value other than Hash.
Originally, the data represented by JSON was not limited to Hash.

https://www.rfc-editor.org/rfc/rfc8259#section-2

The current implementation of JSONParser does not appear to have sufficiently considered this.

We need to consider carefully the specification of the case where raw data can not be parsed as Hash.

The currently intended usecases appears to be Hash or Array of Hash.
We should consider only these 2 patterns as parsable for now.

in_http assumed that Parser could return Array.
Therefore, it needed to be fixed along with this.

MessagePackParser appears to have the same issue. (Not fixed in this PR.)

Docs Changes:
Not needed.

Release Note:
Same as the title.

Difference

Case: No subsequent processing

Config

<source>
  @type tcp
  tag test.tcp
  <parse>
    @type json
  </parse>
</source>

<match test.**>
  @type stdout
</match>

Operation

$ netcat 0.0.0.0 5170
[{"k":"v"}, {"k2":"v2"}]

Result before this fix

2023-03-21 23:30:30.623063804 +0900 test.tcp: [{"k":"v"},{"k2":"v2"}]

Result after this fix

2023-03-21 23:29:20.147728222 +0900 test.tcp: {"k":"v"}
2023-03-21 23:29:20.147754433 +0900 test.tcp: {"k2":"v2"}

Case: With subsequent filter processing

Config

<source>
  @type tcp
  tag test.tcp
  <parse>
    @type json
  </parse>
</source>

<filter test.**>
  @type record_transformer
  enable_ruby true
  <record>
    class ${record.class}
  </record>
</filter>

<match test.**>
  @type stdout
</match>

Operation

$ netcat 0.0.0.0 5170
[{"k":"v"}, {"k2":"v2"}]

Result before this fix

2023-03-21 23:30:51 +0900 [warn]: #0 dump an error event: error_class=NoMethodError error="undefined method `merge!' for [{\"k\"=>\"v\"}, {\"k2\"=>\"v2\"}]:Array" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_record_transformer.rb:135:in `reform'" tag="test.tcp" time=2023-03-21 23:30:51.040692881 +0900 record=[{"k"=>"v"}, {"k2"=>"v2"}]

Result after this fix

2023-03-21 23:29:55.783183244 +0900 test.tcp: {"k":"v","class":"Hash"}
2023-03-21 23:29:55.783215076 +0900 test.tcp: {"k2":"v2","class":"Hash"}

daipom · 2023-03-21T14:57:23Z

Additional confirming of the difference of the behavior,

<source>
  @type sample
  tag test.hash
  sample {"message": "{\"k\":\"v\"}"}
</source>

<source>
  @type sample
  tag test.string
  sample {"message": "\"HELLO\""}
</source>

<source>
  @type sample
  tag test.invalid
  sample {"message": "HELLO"}
</source>

<source>
  @type sample
  tag test.array
  sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"}
</source>

<filter test.**>
  @type parser
  key_name message
  <parse>
    @type json
  </parse>
</filter>

<filter test.**>
  @type record_transformer
  enable_ruby true
  <record>
    class ${record.class}
  </record>
</filter>

<match test.**>
  @type stdout
</match>

Before this fix

Some patterns result in errors in subsequent processing.

2023-03-21 23:50:24.068859931 +0900 test.hash: {"k":"v","class":"Hash"}
2023-03-21 23:50:25 +0900 [warn]: #0 dump an error event: error_class=NoMethodError error="undefined method `merge!' for [{\"k\"=>\"v\"}, {\"k2\"=>\"v2\"}]:Array" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_record_transformer.rb:135:in `reform'" tag="test.array" time=2023-03-21 23:50:25.070141860 +0900 record=[{"k"=>"v"}, {"k2"=>"v2"}]
2023-03-21 23:50:25 +0900 [warn]: #0 dump an error event: error_class=Fluent::Plugin::Parser::ParserError error="pattern not matched with data 'HELLO'" location=nil tag="test.invalid" time=2023-03-21 23:50:25.071323231 +0900 record={"message"=>"HELLO"}
2023-03-21 23:50:25 +0900 [warn]: #0 dump an error event: error_class=NoMethodError error="undefined method `merge!' for \"HELLO\":String" location="/home/daipom/work/fluentd/fluentd/lib/fluent/plugin/filter_record_transformer.rb:135:in `reform'" tag="test.string" time=2023-03-21 23:50:25.072303009 +0900 record="HELLO"

After this fix

Correctly, ParserError occurs in filter_parser.

I left a comment in the code about filter_parser not being able to parse multiple records from one raw record.
(This is a limitation of the current specification.)

2023-03-21 23:43:25.061681273 +0900 test.array: {"k":"v","class":"Hash"}
2023-03-21 23:43:25 +0900 [warn]: #0 dump an error event: error_class=Fluent::Plugin::Parser::ParserError error="pattern not matched with data 'HELLO'" location=nil tag="test.invalid" time=2023-03-21 23:43:25.062907066 +0900 record={"message"=>"HELLO"}
2023-03-21 23:43:25.063493027 +0900 test.hash: {"k":"v","class":"Hash"}
2023-03-21 23:43:25 +0900 [warn]: #0 dump an error event: error_class=Fluent::Plugin::Parser::ParserError error="pattern not matched with data '\"HELLO\"'" location=nil tag="test.string" time=2023-03-21 23:43:25.063894687 +0900 record={"message"=>"\"HELLO\""}

daipom · 2023-03-21T15:01:55Z

Sorry for the timing of the v1.16.0 release.

I understand that we would not be able to put this fix in v1.16.0.

lib/fluent/plugin/in_http.rb

daipom · 2023-03-21T15:14:48Z

lib/fluent/plugin/in_http.rb

      else
        raise "'json', 'ndjson' or 'msgpack' parameter is required"
      end
    end

    def parse_params_with_parser(params)
      if content = params[EVENT_RECORD_PARAMETER]
-        @custom_parser.parse(content) { |time, record|
-          raise "Received event is not #{@format_name}: #{content}" if record.nil?


Explanation of the diff

There did not seem to be any particular reason for raise only here.
We should align with parse_params_default.

test/plugin/test_parser_json.rb

daipom · 2023-03-24T07:45:18Z

I confirmed the failing tests. They seem to be unstable tests and not related to this fix.

ashie · 2023-03-27T03:21:37Z

lib/fluent/plugin/in_http.rb

+          if record.is_a?(Array)
+            # TODO: Temporarily supporting this case for compatibility.
+            # We should not consider this case here.
+            # We should fix MessagePackParser so that it doesn't return Array.


Why don't you do this in this PR?

I thought it should be summarized as fixing the JSON parser.
Since there is no time before the next release, I also wanted to keep the scope of impact as small as possible.
About in_http, I had no choice but to fix it in this PR.

If it looks like I should fix it in this PR, I will try today or tomorrow.
Or I can try to fix it in another PR after this PR is merged.

It should be done in this PR instead of adding such workaround, since it's considered as a same problem and sometimes such workaround lives long unexpectedly.

I’ll postpone merging this to v1.17.0.

Thanks for your review. I Understood.
My understanding was that the existing workaround was minimized here, not adding a new workaround.
But certainly, if it is not fixed entirely, it may lead to unnecessary problems.
Especially for MessagePackParser as @custom_parser.

daipom · 2023-04-10T09:56:57Z

TODO: Fix other parsers.

daipom · 2023-09-12T08:30:46Z

Sorry for the delay. I'm going to fix this for the next release.

It is wrong for Parser to return a record that is not Hash. Subsequent processing may result in errors. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

in_http didn't support yield of Parser. The specification assumed that Parser could return Array. However, this is wrong. Parser shouldn't return Array. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

@type

Config to reproduce: <source> @type sample tag test.array sample {"message": "[{\"k\":\"v\"}, {\"k2\":\"v2\"}]"} </source> <filter test.**> @type parser key_name message <parse> @type json </parse> </filter> <match test.**> @type stdout </match> Result: 2023-03-21 23:24:52.004470792 +0900 test.array: {"k":"v"} 2023-03-21 23:24:52.004470792 +0900 test.array: {"k":"v"} ... Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

daipom · 2024-04-26T10:15:06Z

I have add new PR for this

Make sure parser returns hash #4474

daipom mentioned this pull request Mar 21, 2023

JSON parser plugin can sometimes output records as strings rather than Hash #4100

Open

daipom marked this pull request as draft March 21, 2023 10:04

daipom force-pushed the make-sure-json-parser-returns-hash branch from 9c0005e to f0591f0 Compare March 21, 2023 14:31

daipom marked this pull request as ready for review March 21, 2023 14:58

daipom commented Mar 21, 2023

View reviewed changes

lib/fluent/plugin/in_http.rb Show resolved Hide resolved

daipom commented Mar 21, 2023

View reviewed changes

abetomo reviewed Mar 24, 2023

View reviewed changes

test/plugin/test_parser_json.rb Outdated Show resolved Hide resolved

daipom force-pushed the make-sure-json-parser-returns-hash branch from f0591f0 to 39b72c6 Compare March 24, 2023 07:11

ashie reviewed Mar 27, 2023

View reviewed changes

ashie added this to the v1.17.0 milestone Mar 28, 2023

daipom marked this pull request as draft April 10, 2023 09:56

daipom added 3 commits April 4, 2024 16:33

Make sure JSON parser returns Hash

d5c32e8

It is wrong for Parser to return a record that is not Hash. Subsequent processing may result in errors. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

in_http supports Parser yield

9516ef4

in_http didn't support yield of Parser. The specification assumed that Parser could return Array. However, this is wrong. Parser shouldn't return Array. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

kenhys force-pushed the make-sure-json-parser-returns-hash branch from 39b72c6 to 940624e Compare April 4, 2024 07:33

daipom mentioned this pull request Apr 26, 2024

Make sure parser returns hash #4474

Merged

daipom closed this Apr 26, 2024

daipom deleted the make-sure-json-parser-returns-hash branch May 7, 2024 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure JSON parser returns Hash #4106

Make sure JSON parser returns Hash #4106

daipom commented Mar 21, 2023 •

edited

Loading

daipom commented Mar 21, 2023

daipom commented Mar 21, 2023

daipom Mar 21, 2023 •

edited

Loading

daipom commented Mar 24, 2023

ashie Mar 27, 2023

daipom Mar 27, 2023

ashie Mar 28, 2023

daipom Mar 28, 2023

daipom commented Apr 10, 2023

daipom commented Sep 12, 2023

daipom commented Apr 26, 2024

Make sure JSON parser returns Hash #4106

Make sure JSON parser returns Hash #4106

Conversation

daipom commented Mar 21, 2023 • edited Loading

Difference

Case: No subsequent processing

Case: With subsequent filter processing

daipom commented Mar 21, 2023

Before this fix

After this fix

daipom commented Mar 21, 2023

daipom Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

daipom commented Mar 24, 2023

ashie Mar 27, 2023

Choose a reason for hiding this comment

daipom Mar 27, 2023

Choose a reason for hiding this comment

ashie Mar 28, 2023

Choose a reason for hiding this comment

daipom Mar 28, 2023

Choose a reason for hiding this comment

daipom commented Apr 10, 2023

daipom commented Sep 12, 2023

daipom commented Apr 26, 2024

daipom commented Mar 21, 2023 •

edited

Loading

daipom Mar 21, 2023 •

edited

Loading